🔍 Code Review Summary

This PR successfully refactors FabricFrameView to consolidate duplicate kernel-launch logic into a single _compose_fabric_transform method and addresses a critical assertion bug. The changes are well-structured and improve both correctness and maintainability.

✅ Correctness

No blocking issues found.

The refactoring maintains behavioral equivalence:

• _compose_fabric_transform correctly handles None parameters via empty arrays, preserving the kernel's ability to skip unspecified components
• The _fabric_usd_sync_done flag is set correctly after writes
• The assert → RuntimeError change in _rebuild_fabric_arrays is a valuable fix — assertions are stripped under python -O, which could silently produce wrong poses or out-of-bounds kernel indices in production

Minor observation:

• The _compose_fabric_transform method always calls _fabric_hierarchy.update_world_xforms() even when only scales are updated. This matches the original set_scales behavior, so no regression, but worth noting for future optimization.

⚡ Performance

Net positive impact:

1. Reduced PrepareForReuse calls — The critical fix: _sync_fabric_from_usd_once now invokes PrepareForReuse exactly once (via the fused compose) instead of twice. This prevents potential topology-change signal masking.

2. Single kernel launch for initial sync — USD→Fabric sync now uses one wp.launch instead of two, reducing kernel launch overhead.

Micro-optimization opportunity (non-blocking):

# Current: creates empty arrays every call
empty3 = wp.zeros((0, 3), dtype=wp.float32, device=self._device)
empty4 = wp.zeros((0, 4), dtype=wp.float32, device=self._device)

These could be cached as class-level constants, but the overhead is negligible for zero-sized arrays.

📝 Maintainability

Excellent improvements:

1. Code deduplication — ~40 lines of duplicate kernel-launch boilerplate consolidated into one reusable method
2. Clearer naming — empty3/empty4 communicates intent better than dummy3/dummy4
3. Good documentation — The _compose_fabric_transform docstring clearly explains the single-PrepareForReuse guarantee and the _fabric_usd_sync_done side effect
4. Test modernization — wp.to_torch(fab_pos) → fab_pos.torch uses the current ProxyArray API

Changelogs are well-formatted and accurately describe both the behavioral changes and the assert fix.

🔗 Service Locator (Bundled Feature)

The typed service locator in SimulationContext is a clean pattern:

• Type-safe via TypeVar
• Automatic close() on clear_instance()
• Good test coverage for edge cases (re-registration, missing close method, etc.)

One consideration: set_service does not auto-close the replaced service (caller responsibility per docstring). This is documented but could be a subtle footgun. Consider adding an optional auto_close=True parameter in the future if replacement becomes common.

Verdict: Approve ✅

The refactoring achieves its goals cleanly:

• Eliminates duplicate code
• Fixes a real bug (assert under -O)
• Reduces PrepareForReuse call count
• Adds useful infrastructure (service locator)

All 36 existing Fabric tests pass per PR description. Ship it! 🚀

🔍 Code Review Summary

This PR successfully refactors FabricFrameView to consolidate duplicate kernel-launch logic into a single _compose_fabric_transform method and moves Fabric/USD dispatch to the FrameView factory. The changes improve both correctness and maintainability.

✅ Correctness

No blocking issues found.

The refactoring maintains behavioral equivalence:

• _compose_fabric_transform correctly handles None parameters via empty arrays, preserving the kernel's ability to skip unspecified components
• The _fabric_usd_sync_done flag is correctly set after writes
• The assert → RuntimeError change in _rebuild_fabric_arrays is a valuable fix — assertions are stripped under python -O, which could silently produce wrong poses or out-of-bounds kernel indices in production
• Factory dispatch logic in FrameView._get_backend() correctly checks both fabricEnabled setting and device support before routing to FabricFrameView

Minor observation:

• The _compose_fabric_transform method always calls _fabric_hierarchy.update_world_xforms() even when only scales are updated. This matches the original set_scales behavior, so no regression, but worth noting for future optimization.

⚡ Performance

Net positive impact:

1. Reduced PrepareForReuse calls — The critical fix: _sync_fabric_from_usd_once now invokes PrepareForReuse exactly once (via the fused compose) instead of twice. This prevents potential topology-change signal masking.

2. Single kernel launch for initial sync — USD→Fabric sync now uses one wp.launch instead of two, reducing kernel launch overhead.

Micro-optimization opportunity (non-blocking):

# Current: creates empty arrays every call
empty3 = wp.zeros((0, 3), dtype=wp.float32, device=self._device)
empty4 = wp.zeros((0, 4), dtype=wp.float32, device=self._device)

These could be cached as class-level constants, but the overhead is negligible for zero-sized arrays.

📝 Maintainability

Excellent improvements:

1. Code deduplication — ~40 lines of duplicate kernel-launch boilerplate consolidated into one reusable method
2. Single-responsibility — FabricFrameView no longer contains dispatch logic; it assumes Fabric is available (factory guarantees this)
3. Clearer naming — empty3/empty4 communicates intent better than dummy3/dummy4
4. Good documentation — The _compose_fabric_transform docstring clearly explains the single-PrepareForReuse guarantee and the _fabric_usd_sync_done side effect
5. Test modernization — wp.to_torch(fab_pos) → fab_pos.torch uses the current ProxyArray API

Changelogs are well-formatted and accurately describe both the behavioral changes and the assert fix.

🏭 Factory Dispatch (frame_view.py)

The dispatch logic is clean and well-structured:

• FrameView._get_backend() checks SimulationContext for physics manager type
• PhysX path now explicitly checks fabricEnabled setting and device compatibility
• UsdFrameView is eagerly registered since it lives in isaaclab, not a backend package
• Good warning message when Fabric is enabled but device is unsupported

One nit (non-blocking): The _FABRIC_SUPPORTED_DEVICES tuple could potentially be shared between frame_view.py and any other code that needs it (currently duplicated between factory and docs), but this is minor.

⚠️ CI Status

The "Check changelog fragments" job is failing. Please verify the changelog fragment format matches project conventions.

Verdict

The refactoring achieves its goals cleanly:

• ✅ Eliminates duplicate kernel-launch code
• ✅ Fixes a real bug (assert stripped under -O)
• ✅ Reduces PrepareForReuse call count in initial sync
• ✅ Moves dispatch responsibility to the factory (single-responsibility principle)
• ✅ All 36 existing Fabric tests pass per PR description

Once the changelog CI check passes, this is ready to merge. 🚀